[Xamarin.Android.Build.Tasks] Bump ZipFlushFilesLimit #7957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Comment on lines
11
to
12
| public static int ZipFlushSizeLimit = 50 * 1024 * 1024; | ||
| public static int ZipFlushFilesLimit = 50; | ||
| public static int ZipFlushSizeLimit = 100 * 1024 * 1024; | ||
| public static int ZipFlushFilesLimit = 512; |
There was a problem hiding this comment.
Can we make this a private MSBuild property? $(_AndroidZipFlushSizeLimit) and $(_AndroidZipFlushFilesLimit) would give us the ability to tell customers about it. And see if that fixes an issue for them?
There was a problem hiding this comment.
hmm, that might be a good idea. 🤔
There was a problem hiding this comment.
While contemplating that question, why are these fields read+write? We don't ever change them, so shouldn't they be const?
There was a problem hiding this comment.
The idea at the start was probably that we could alter the defaults in code. But we never did lol
There was a problem hiding this comment.
The idea at the start was probably that we could alter the defaults in code. But we never did lol
…but you are now, in e.g. AddEntryAndFlush(). Which means we now have unprotected globally mutable data, which I am allergic to.
For example -- and because I cannot remember -- what happens in a "redux"/"variant" of 22f10b2 (our favorite issue around .GetRegisteredTaskObjectAssemblyLocal<T>() now needing to include a directory as part of the key to prevent unexpected sharing of data between project builds) and we have 2 or more projects which just happen to hit <BuildApk/> at the "same" time and "just happen" to provide different values for $(_ZipFlushFilesLimit)?
It might be "fine" but I don't like it.
I would prefer it if these fields became instance values set at ZipArchiveEx construction time:
Context https://i.azdo.io/1782014 On Windows customers are seeing the following error ``` Error XABLD7000: Xamarin.Tools.Zip.ZipException: Renaming temporary file failed: Permission denied at Xamarin.Tools.Zip.ZipArchive.Close() in /Users/runner/work/1/s/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs:line 939 ``` We have traced this to the use of the `MoveFileExW`/`MoveFileExA` API. When libzip is trying to move its temp file to the final location, Windows is raising this error. ``` Renaming temporary file failed: Permission denied ``` Turning off Anti Virus seems to help, however adding an exclusion does not. This is very confusing. So we are unsure why this error is being raised. The process has the correct permissions and the file being moved is in the same directory, so its not a TEMP folder issue. So perhaps its the number of temp files we create? Part of the `BuildApk` system is that as we add files we `Flush` the zip file to commit those changes to disk. This is partly to work around how libzip works. It does not write any data to the main file until `zip_close` is called. So to work around issues around too many files being open [1], we added this flush. The limit of 50 files was picked out of a hat. So lets try pushing the limit up a bit to see if that helps. [1] dotnet@9166e03
dellis1972
force-pushed
the
libzippermerror
branch
from
2940d2e
to
99e8a95
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jonathanpeppers
approved these changes
Apr 18, 2023
|
Draft commit message: [Xamarin.Android.Build.Tasks] Bump ZipFlushFilesLimit (#7957)
Context: https://dev.azure.com/DevDiv/DevDiv/_workitems/edit/1782014
Context: 9166e0363417d6d121de37a765db8f2ba7cece7b
On Windows customers are seeing the following error:
error XABLD7000: Xamarin.Tools.Zip.ZipException: Renaming temporary file failed: Permission denied
at Xamarin.Tools.Zip.ZipArchive.Close() in /Users/runner/work/1/s/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs:line 939
We have traced this to the use of the [`MoveFileExW()`][0] /
`MoveFileExA()` API: when libzip is trying to move its temp file to
the final location, Windows is raising this error:
Renaming temporary file failed: Permission denied
Turning off Anti Virus seems to help, however adding an exclusion
does not. This is very confusing, so we are unsure why this error is
being raised. The process has the correct permissions and the file
being moved is in the same directory, so its not a TEMP folder issue.
Perhaps it's the number of temp files we create? Part of the
`<BuildApk/>` system is that as we add files we `Flush()` the zip
file to commit those changes to disk. This is partly to work around
how libzip works: it does not write any data to the main file until
[`zip_close()`][1] is called. To work around issues around too many
files being open (9166e036), we added this flush.
The limit of 50 files was picked out of a hat. Try pushing the limit
up a bit to see if that helps.
Additionally, introduce the following two (private!) MSBuild
properties:
* `$(_ZipFlushFilesLimit)`: Call `Flush()` after
`$(_ZipFlushFilesLimit)` files have been added to the `.apk`.
* `$(_ZipFlushSizeLimit)`: Call `Flush()` after
`$(_ZipFlushSizeLimit)` bytes of data have been added to the
`.apk`.
[0]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw
[1]: https://libzip.org/documentation/zip_close.html |
grendello
added a commit
to grendello/xamarin-android
that referenced
this pull request
Apr 24, 2023
* main: [Xamarin.Android.Build.Tasks] enable ForceInterpretedInvoke switch (dotnet#7972) [Mono.Android] Bind API-UpsideDownCake Beta 1 (dotnet#7980) Bump to xamarin/xamarin-android-tools/main@8bc07503 (dotnet#7863) [automation] Add 'xaSourcePath' to yaml so they can be used from monodroid. (dotnet#7978) Bump to dotnet/installer@16c10f8115 8.0.100-preview.4.23218.1 (dotnet#7969) [docs] Add UnitTest.md (dotnet#7877) [ci] Suppress fork PR build warnings (dotnet#7973) [Xamarin.Android.Build.Tasks] Bump ZipFlushFilesLimit (dotnet#7957) Bump to dotnet/installer@3ca7ad1c79 8.0.100-preview.4.23211.1 (dotnet#7946) [CI] Allow passing xamarin-android checkout dir to nested templates. (dotnet#7961) [Xamarin.Android.Build.Tasks] Fix `-int.ToString()` for locales (dotnet#7941) [ci] Automatically retry failed apk-instrumentation tests. (dotnet#7963)
jonathanpeppers
pushed a commit
that referenced
this pull request
May 25, 2023
Context: https://dev.azure.com/DevDiv/DevDiv/_workitems/edit/1782014 Context: 9166e03 On Windows customers are seeing the following error: error XABLD7000: Xamarin.Tools.Zip.ZipException: Renaming temporary file failed: Permission denied at Xamarin.Tools.Zip.ZipArchive.Close() in /Users/runner/work/1/s/LibZipSharp/Xamarin.Tools.Zip/ZipArchive.cs:line 939 We have traced this to the use of the [`MoveFileExW()`][0] / `MoveFileExA()` API: when libzip is trying to move its temp file to the final location, Windows is raising this error: Renaming temporary file failed: Permission denied Turning off Anti Virus seems to help, however adding an exclusion does not. This is very confusing, so we are unsure why this error is being raised. The process has the correct permissions and the file being moved is in the same directory, so its not a TEMP folder issue. Perhaps it's the number of temp files we create? Part of the `<BuildApk/>` system is that as we add files we `Flush()` the zip file to commit those changes to disk. This is partly to work around how libzip works: it does not write any data to the main file until [`zip_close()`][1] is called. To work around issues around too many files being open (9166e03), we added this flush. The limit of 50 files was picked out of a hat. Try pushing the limit up a bit to see if that helps. Additionally, introduce the following two (private!) MSBuild properties: * `$(_ZipFlushFilesLimit)`: Call `Flush()` after `$(_ZipFlushFilesLimit)` files have been added to the `.apk`. * `$(_ZipFlushSizeLimit)`: Call `Flush()` after `$(_ZipFlushSizeLimit)` bytes of data have been added to the `.apk`. [0]: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw [1]: https://libzip.org/documentation/zip_close.html
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context https://i.azdo.io/1782014
On Windows customers are seeing the following error
We have traced this to the use of the
MoveFileExW/MoveFileExAAPI. When libzip is trying to move its temp file to the final location, Windows is raising this error.Turning off Anti Virus seems to help, however adding an exclusion does not. This is very confusing. So we are unsure why this error is being raised. The process has the correct permissions and the file being moved is in the same directory, so its not a TEMP folder issue.
So perhaps its the number of temp files we create? Part of the
BuildApksystem is that as we add files weFlushthe zip file to commit those changes to disk. This is partly to work around how libzip works. It does not write any data to the main file untilzip_closeis called. So to work around issues around too many files being open [1], we added this flush.The limit of 50 files was picked out of a hat. So lets try pushing the limit up a bit to see if that helps.
[1] 9166e03